wp_drbg: gate wc_RNG_DRBG_Reseed on FIPS version#408
Conversation
wc_RNG_DRBG_Reseed is WOLFSSL_LOCAL in FIPS v5.2.1 (cert4718), causing an undefined-reference link failure; it is public in non-FIPS >= 5.7.2 and FIPS v5.2.4/v6/v7. Gate WP_HAVE_DRBG_RESEED on the FIPS version in settings.h and reseed in place when public, else re-instantiate. Also map --fips-check=v5.2.4 to --enable-fips=v5.2.4 instead of collapsing to v5.
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-security + bugsOverall recommendation: COMMENT
Findings: 5 total — 4 posted, 1 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review+review-security] FIPS fallback reseed re-instantiates from OS entropy, bypassing the parent-seed/seccomp design and treating caller entropy as nonce —
src/wp_drbg.c:440-485 - [Medium] [review] New error-state and explicit-entropy reseed paths have no test coverage —
src/wp_drbg.c:328-333,440-485,612-644 - [Low] [review-security+bugs] wp_drbg_get_seed does not check the new rngError state, operating on a de-instantiated RNG after a failed fallback reseed —
src/wp_drbg.c:734-756 - [Low] [review] wc_FreeRng() return code ignored in fallback reseed —
src/wp_drbg.c:468
Skipped findings
- [Low]
utils-wolfssl.sh adds untested v5.2.3 - --enable-fips=v5.2.3 mapping that changes prior behavior
Review generated by Skoll
Draw fresh entropy via wp_urandom_read (cached /dev/urandom fd, survives a seccomp sandbox) on the native path when the caller supplies none; the cert4718 fallback self-seeds via wc_InitRngNonce. Neither path opens a file during reseed. Fixes null-entropy handling the fallback previously skipped, and guards wp_drbg_get_seed while the DRBG is in an error state.
…y bundle) wc_RNG_DRBG_Reseed visibility is not predictable from the FIPS version: across frozen commercial bundles the same v5.2.4 cert is WOLFSSL_LOCAL on a 5.8.4 wrapper but WOLFSSL_API on a 5.9.1 wrapper, so the prior "v5.2.4+ -> native" gate hit `undefined reference to wc_RNG_DRBG_Reseed` on 5.8.4+v5.2.4. Use the native in-place reseed only where the symbol is reliably exported (non-FIPS >= 5.7.2, FIPS v6+) and fall back to DRBG re-instantiation for all FIPS v5.x. Add WP_NO_DRBG_RESEED to force the fallback for any bundle that keeps the symbol WOLFSSL_LOCAL despite its version. Verified: builds + full unit suite pass on 5.8.4/5.9.1 wrappers x v5.2.1/v5.2.4 commercial FIPS bundles (5/5, 136/0).
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
Skoll Multi-Scan Review
Modes: review + review-securityOverall recommendation: COMMENT
Findings: 6 total — 4 posted, 2 skipped
4 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] [review] Untested v5.2.3 FIPS tag mapping added without validation —
scripts/utils-wolfssl.sh:186-188 - [Medium] [review] Bare scope block introduced in wp_drbg_reseed native path —
src/wp_drbg.c:400-455 - [Low] [review] Non-ASCII em-dash in shell comment —
scripts/utils-wolfssl.sh:183 - [Low] [review-security] Bare scope block introduced in wp_drbg_reseed native reseed path —
src/wp_drbg.c:400-455
Skipped findings
- [Info]
FIPS v5.x fallback reseed deviates from SP 800-90A reseed semantics - [Low]
Missing unit coverage for fallback reseed error-state path
Review generated by Skoll
| # Distinct module (SP math, PATCH 4) — not the v5/cert4718 base | ||
| fips_configure_arg="v5.2.4" | ||
| ;; | ||
| v5.2.3|linuxv5.2.3) |
There was a problem hiding this comment.
🟠 [Medium] Untested v5.2.3 FIPS tag mapping added without validation
The PR adds a new case mapping v5.2.3|linuxv5.2.3 to --enable-fips=v5.2.3. The PR description and validation only cover v5.2.1 and v5.2.4 bundles; v5.2.3 is neither mentioned nor tested, and unlike the v5.2.4 case it carries no comment explaining why it is a distinct module. If wolfSSL's configure does not accept --enable-fips=v5.2.3 as a recognized value, builds invoked with that tag will now fail at configure time, whereas before this change they fell through to the working v5 mapping. This silently broadens behavior for an unvalidated tag.
Fix: Confirm --enable-fips=v5.2.3 is a valid configure value for the targeted wolfSSL versions. If it is not validated, drop the v5.2.3 case so it falls through to v5 (the prior, working behavior), or add a comment documenting which cert/module it targets like the v5.2.4 case does.
| ok = 0; | ||
| } | ||
|
|
||
| #ifdef WP_HAVE_DRBG_RESEED |
There was a problem hiding this comment.
🟠 [Medium] Bare scope block introduced in wp_drbg_reseed native path
The #ifdef WP_HAVE_DRBG_RESEED branch opens a standalone { ... } block solely to scope the local declarations unsigned char* seed and size_t seedLen. This is a bare scope block that does not belong to a function, control statement, switch/case, or type definition. The wolfSSL/wolfProvider C style explicitly bans introducing bare scope blocks in new code to make C89 declarations work — declarations should move to the top of the enclosing function (or the logic split into a helper). Note the sibling #else fallback branch correctly declares its locals inside an if (ok) { control block, so the two halves of the same function are inconsistent.
Fix: Remove the bare scope block to comply with the no-bare-scope-block convention; the empty-brace-scan skill flags exactly this pattern.
| local fips_configure_arg="" | ||
| case "$fips_tag" in | ||
| v5.2.4|linuxv5.2.4) | ||
| # Distinct module (SP math, PATCH 4) — not the v5/cert4718 base |
There was a problem hiding this comment.
🔵 [Low] Non-ASCII em-dash in shell comment
The added comment uses a Unicode em-dash (—) in an otherwise ASCII source tree. For consistency with the rest of the C/shell sources and to avoid encoding surprises in tooling/diffs, prefer a plain ASCII hyphen.
Fix: Replace the em-dash with an ASCII hyphen.
| ok = 0; | ||
| } | ||
|
|
||
| #ifdef WP_HAVE_DRBG_RESEED |
There was a problem hiding this comment.
🔵 [Low] Bare scope block introduced in wp_drbg_reseed native reseed path · Logic
The new native (WP_HAVE_DRBG_RESEED) branch opens a standalone { ... } scope solely to declare unsigned char* seed and size_t seedLen mid-function. This is a bare scope block, which the wolfSSL/wolfProvider C coding standard prohibits in new code (scope blocks are only allowed for function/control/switch/type bodies). The parallel fallback branch correctly uses a control-statement block (if (ok) { ... }), so only the native path violates the rule. BEFORE: these two locals were declared at the top of the function. AFTER: they were moved into a new bare { } block under the #ifdef. This is a style/maintainability issue, not a security defect.
Fix: Declare seed/seedLen at the top of wp_drbg_reseed (guarded as needed) and remove the bare { } scope, or move the native logic into a small helper function, to comply with the no-bare-scope coding standard. Run the empty-brace-scan to confirm no other new scope blocks were introduced.
2 participants
wp_drbg_reseed references wc_RNG_DRBG_Reseed, but that is not always an exported symbol. It is WOLFSSL_API in non-FIPS >= 5.7.2 and in FIPS v6+, while FIPS v5.x commercial bundles are inconsistent -- the same v5.2.4 cert is WOLFSSL_LOCAL on a 5.8.4 wrapper but WOLFSSL_API on a 5.9.1 wrapper. Linking it fails (undefined reference to wc_RNG_DRBG_Reseed) on the bundles that keep it LOCAL, which the previous version-only gate did not account for.
Validated: build + full unit suite pass (136/0) against the 5.8.4 and 5.9.1 wolfSSL wrappers x v5.2.1 and v5.2.4 commercial FIPS bundles, plus non-FIPS.
Note on the FIPS v5.x fallback path: caller-supplied entropy/addIn is folded into wc_InitRngNonce as the nonce, not mixed as DRBG entropy_input, and predResist is not honored -- wolfCrypt re-seeds internally via its seed source. This is the available behavior where the module does not export wc_RNG_DRBG_Reseed, and differs from the native path where caller entropy is reseed input.